-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: improve tree-shaking by propagate const parameter #5443
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Update: I think it will be pretty useful for library usage, I have just update an optimization: before: function fun1(options) {
if (options.enable) {
return 'fun1';
} else {
console.log(222);
}
}
function fun2(options) {
if (options.enable) {
return fun1(options);
} else {
console.log(111);
}
}
console.log(
fun2({
enable: true
})
); output: function fun1(options) {
if (options.enable) {
return 'fun1';
}
}
function fun2(options) {
if (options.enable) {
return fun1(options);
}
}
console.log(
fun2({
enable: true
})
); and providing an |
Thank you for your contribution! ❤️You can try out this pull request locally by installing Rollup via npm install liuly0322/_rollup#master Notice: Ensure you have installed Rust nightly. If you haven't installed it yet, please first see https://www.rust-lang.org/tools/install to learn how to download Rustup and install Rust, then see https://rust-lang.github.io/rustup/concepts/channels.html to learn how to install Rust nightly. or load it into the REPL: |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #5443 +/- ##
==========================================
+ Coverage 98.81% 98.83% +0.02%
==========================================
Files 238 238
Lines 9451 9554 +103
Branches 2408 2439 +31
==========================================
+ Hits 9339 9443 +104
+ Misses 47 46 -1
Partials 65 65 ☔ View full report in Codecov by Sentry. |
Thank you for your work! This is a really interesting approach. I will give you a more thorough review in the next days as I find time, maybe we can develop this in a slightly different direction. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, as promised, here is my review. This PR does indeed show quite some ingenuity. It is not free of bugs but that would be fixable. Performance is also ok as your additional pass only scans top-level variables.
What I do not like, though, is that it does not tie well into the existing tree-shaking logic but rather appears to build its separate logic next to it. That also means that it
- needs to make a distinction to work only for top-level variables (which avoids performance issues)
- does not take side effects into account, e.g. here, it could also remove the
if(x.y)
, but it is retained because the property accessx.y
might have a side effect—which it obviously does not have. - I am not sure how easy it is to build other features on top of this.
Also, there already IS a mechanism that connects call arguments and functions. Actually, two mechanisms. One is includeCallArguments
. This one is responsible for removing unused arguments from function calls like here.
this might be one candidate where one could put additional logic for parameter tracking, i.e. the ParameterScope could tell the ParameterVariables which values were included. Note though that this might be a little late as until the point where the call is included, the function was evaluated without the parameter value, which means it can probably only assume the value to be "unknown", which might provide suboptimal results.
The other mechanism is deoptimizeArgumentsOnInteractionAtPath. This one is triggered very early, basically the first time a CallExpression is scanned for effects (which roughly corresponds to the first time Rollup determines this CallExpression to be in an included code path) or the first time it is included (for unconditionally included code), see CallExpression.applyDeoptimizations. This mechanism tracks mutations that happens in functions to mark call arguments as "deoptimized" if necessary, so that e.g. they do not report wrong literal values. You could use deoptimizeArgumentsOnInteractionAtPath
to report possible values to the parameters.
You could then not limit the values to literal values alone, but if you do that, I would make sure not to track long lists of call parameters because then you might run into performance problems. Or again limit yourself to literal values for now. Rollup usually has the logic that if a function is reassigned, the value is assumed to be "unknown" because otherwise, we would often quickly run into exponential performance degradation issues.
There is another case to consider, though. What happens if a function is exported or assigned to a global variable? In that case, we must not make assumption about passed in parameters. Luckily, this case can be easily detected as well.
Would you consider re-implementing the feature in such a direction? One immediate benefit would be that it would not longer be limited to top-level functions and also work e.g. in IIFEs etc.
Thank you very much for your detailed explanation! I didn't know much about the internal details of rollup before, you help me a lot. I have refactored my code. I have updated the logic to fit in the tree-shaking framework. Now the updated version works for old tests, and new tests:
Below is a new problem: When Also, I am not sure how to remove Thank you again! |
getObjectEntity is private, so we can't judge if two object are the same
This PR contains:
Are tests included?
Breaking Changes?
List any relevant issue numbers:
Update
(by @lukastaegert)
This feature has now been fixed (#5481, #5482, #5486), reverted (#5487), re-implemented (#5483) and re-released and fixed again (#5500, #5503) and there have been no new issues for the entire week now, so I think this is really good and stable now. See the original description below.
This feature tracks all arguments with which a function is called to be able to perform side effect detection and tree-shaking within functions itself. If a function is only called once, the argument values will be used as parameter values inside the function to remove dead code branches.
When the function is called several times, it still applies to all parameters that are called with the same identifier in all calls or with variables that can be tracked by Rollup to have the same primitive values (string, boolean, number, null).
Here are some REPL examples to play around with and get a feeling for what it means:
Description
Hi, all! This Pull Request trys to collect all function calls of top-level functions, so that we can determine if a param of the function is a literal to improve tree shaking (or be used to remove unused default params, which is not included in this PR)
The idea comes from #5435 , and related PRs: #4498 , #4510 , which are finally reverted by #4520
This PR has changed the original code framework to some extent (to use a new approach to collect function use-def information), so I want to know whether the method of collecting all references in the PR and the placement position of the optimization pass are reasonable, thanks!
before:
after: